Added method DateTime::createFromImmutable() [follow-up for #1145] #2484

Majkl578 · 2017-04-17T17:18:40Z

This basically resurrects PR #1145 which was initially merged, but then reverted because of some not really relevant objections.

The primary purpose of this method should be interoperability between DateTime and DateTimeImmutable. The further already has createFromMutable method, but the former has none. So if one wants to convert mutable to immutable, it's as easy as:

DateTime::createFromImmutable($dateTime)

but to convert immutable to mutable, it requires annoying boilerplate:

DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())

There are still tons of libraries that rely only on DateTime class (looking at you, Doctrine). Of course, any new code should use the immutable variant, but it's not always feasible as such libraries wouldn't understand the immutable type - therefore it requires conversion.

I still believe it was a wrong decision to revert this before PHP 7, especially because the only arguments against were totally off topic / invalid.
Quoting @derickr's response from the mailing list, which was the only objection as far as I know:

When the factory method was added, we had this same discussion. And the
discussions lead to the conclusion that it does not make sense to have
this "createFromImmutable" factory method on DateTime. Basically with
the same reasons that people should type hint against the Interface
instead. To emulate that in PHP 5.5 and lower, users can just do:

class DateTimeInterface extends DateTime {}

I know, that's a bit of a hack.

These arguments are totally irrelevant. This is not a factory method, it's a method for interoperability and compatibility. Pretty much like it'd be $mutable->toImmutable() or $immutable->toMutable().
Also the hack is totally meaningless and kind of funny, since the libraries that use DateTime are obviously doing it for purpose (i.e. backwards compatibility - semver anyone?).

I'd really like to see this resolved befrore PHP 7.2. It has already been blocked for two years and @derickr didn't even bother to reply to objections on the original PR...

derickr · 2017-04-17T17:33:10Z

ext/date/php_date.c

-	PHP_ME(DateTime,			__set_state,		NULL, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
+	PHP_ME(DateTime,			__construct,			arginfo_date_create, ZEND_ACC_CTOR|ZEND_ACC_PUBLIC)
+	PHP_ME(DateTime,			__wakeup,				NULL, ZEND_ACC_PUBLIC)
+	PHP_ME(DateTime,			__set_state,			NULL, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)


Don't change white space with functional changes.

Was just following the alignment, but ok, reverted.

derickr · 2017-04-17T17:34:43Z

ext/date/tests/DateTime_verify.phpt

@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) {
  string(8) "DateTime"
 }
 ..and get names of all its methods
-array(18) {
+array(19) {


What's the point of this test? (I realise, it has nothing to do with the PR). I'd say we remove it as it just tests that Reflection works.

I was asking the same question myself, but just cowardly changed the expectf, because I didn't want to remove tests in the PR.

This test is extremely important, since reflection of internal classes is a constant SNAFU

derickr · 2017-04-17T17:37:27Z

ext/date/php_date.c

+	}
+	if (old_obj->time->tz_info) {
+		new_obj->time->tz_info = old_obj->time->tz_info;
+	}


These lines (2842-2849) should be replaced by using timelib_time_clone.

Done, also changed createFromMutable to do so (which I simply duplicated).

Majkl578 · 2017-04-17T17:56:52Z

@derickr Thanks for your review, adressed your comments. What do you think about adding this method now, two years later? I still think it'd be a useful addition for interoperability between mutable & immutable datetimes. ☺️

Majkl578 · 2017-04-17T19:40:16Z

Travis failure seems unrelated:

The job exceeded the maximum time limit for jobs, and has been terminated.

hikari-no-yume · 2017-04-17T20:18:10Z

but to convert immutable to mutable, it requires annoying boilerplate:

DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())

I think that boilerplate is, in itself, a good reason to add this. It's an unfortunately not-uncommon thing to need to do, and I imagine it would be easy to mess up (losing or mangling data) if you do it yourself, as well as being plain inefficient.

enumag · 2017-04-17T21:52:52Z

@Majkl578 In my opinion it has very limited use unless it is createFromInterface implemented on both DateTime and DateTimeImmutable.

Majkl578 · 2017-04-17T23:42:44Z

@enumag From the point of the interface, it doesn't make much sense. Since the interface doesn't know its implementors (OO principle), it could only accept itself and return itself.
So the signature would look as such (simplified code):

ínterface DateTimeInterface {
    static function createFromInterface(DateTimeInterface $other) : DateTimeInterface;
}

Therefore it would be in no way different from __clone (just static). Besides, it even looks strange.

Also, since the implementors can't introduce more specific return types (return types are invariant), they could only return the interface as well (doesn't help static analysis).
Similar for parameter types, implementors can't introduce more specific parameter types (which would violate LSP), they would be forced to accept itself as well (doesn't help static analysis either).
Because of the points above, such method would serve two purposes: a) clone/duplicate itself (or throw exception, which would be strange) and b) reinterpret the sibling.
I'm generally -1 on such overly generic method.

derickr

@Majkl578:

@derickr Thanks for your review, adressed your comments. What do you think about adding this method now, two years later? I still think it'd be a useful addition for interoperability between mutable & immutable datetimes. ☺️

I don't care enough about this method to be here or not.

But I did add some more comments to the code.

derickr · 2017-04-18T09:58:10Z

NEWS

@@ -51,6 +51,8 @@ PHP                                                                        NEWS
  . Fixed bug #74404 (Wrong reflection on DateTimeZone::getTransitions).
    (krakjoe)
  . Fixed bug #74080 (add constant for RFC7231 format datetime). (duncan3dc)
+  . Added DateTime::createFromImmutable() method.
+    (bugs dot php dot net at majkl578 dot cz, Rican7)


There should be an issue for this, and the issue should be mentioned in the NEWS file, just like the others.

Created one: https://bugs.php.net/bug.php?id=74668

derickr · 2017-04-18T09:59:28Z

ext/date/php_date.c

@@ -2816,7 +2822,28 @@ PHP_METHOD(DateTimeImmutable, __construct)
 }
 /* }}} */

-/* {{{ proto DateTimeImmutable::createFromMutable(DateTimeZone object)
+/* {{{ proto DateTime::createFromImmutable(DateTimeImmutable object)
+   Creates new DateTime object from an existing immutable DateTimeImmutable object.


Would it not make more sense to group this with the other DateTime methods, instead of just before the DateTimeImmutable/createFromMutable method?

Where exactly should I put it then? There are __construct for mutable & immutable together, __set_state for both together as well...

derickr · 2017-04-18T09:59:53Z

ext/date/tests/DateTime_verify.phpt

@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) {
  string(8) "DateTime"
 }
 ..and get names of all its methods
-array(18) {
+array(19) {


enumag · 2017-04-18T13:53:39Z

@Majkl578 I didn't mean adding the method to the ineterface. I meant this:

class DateTime implements DateTimeInterface {
    static function createFromInterface(DateTimeInterface $other) : DateTime {...}
}
class DateTimeImmutable implements DateTimeInterface {
    static function createFromInterface(DateTimeInterface $other) : DateTimeImmutable {...}
}

drAlberT · 2017-05-19T23:08:13Z

hope we will see the light .. ;(

DennisBirkholz · 2017-05-28T12:15:54Z

I really don't like this (as I don't like the createFromMutable() method). Consider the following example:

<?php
    function doDomainSpecificTransformationWithDate(\DateTimeInterface $originalDate) {
        if ($originalDate instanceof \DateTimeImmutable) {
            $changeableDate = \DateTime::createFromImmutable($originalDate);
        } else {
            $changeableDate = clone $originalDate;
        }
        ...
    }

So the \DateTimeInterface typehint does not help you at all as you always have to distinguish between all available implementations. I prefer the solution suggested by @enumag even though I don't really like the method name. Maybe createFromObject or createFromDate or createFromOther?

Majkl578 · 2017-05-28T21:07:53Z

I didn't mean adding the method to the interface.

If you don't, the call wouldn't be type safe (any static analyser like PHPStan would fail otherwise due to undefined method) and you would end up with instanceof check either DateTime or DateTimeImmutable anyway.

So the \DateTimeInterface typehint does not help you at all as you always have to distinguish between all available implementations.

This is correct. Most of the time when you need this sort of methods is a compatibility between legacy and modern code, where you'd be using a concrete type anyway. This is also why the argument and return type are not DateTimeInterface.

As I've already said, it doesn't make sense to make a generic method for both DateTime and DateTiumeImmutable in the interface. And even if we did, the method would work in a half-redundant way - it would either transform the other type or duplicate __clone.

nikic · 2017-06-23T12:03:01Z

@derickr Is this okay to merge?

derickr · 2017-07-03T11:46:29Z

@nikic I don't particularly have an opinion, but I think it would be good if somebody would look at this (@Ocramius perhaps?) to validate the ideas about the typehint continuing to work, and this not going to cause any other API/compat issues.

Ocramius · 2017-07-03T11:56:07Z

ext/date/tests/DateTime_verify.phpt

@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) {
  string(8) "DateTime"
 }
 ..and get names of all its methods
-array(18) {
+array(19) {


This test is extremely important, since reflection of internal classes is a constant SNAFU

Ocramius · 2017-07-03T11:56:48Z

ext/date/php_date.h

@@ -53,6 +53,7 @@ PHP_FUNCTION(getdate);
 PHP_METHOD(DateTime, __construct);
 PHP_METHOD(DateTime, __wakeup);
 PHP_METHOD(DateTime, __set_state);
+PHP_METHOD(DateTime, createFromImmutable);
 PHP_FUNCTION(date_create);


I would say fromImmutable, since you aren't really creating it, but rather casting it

True, but there is already DateTimeImmutable::createFromMutable(), so the naming is chosen mostly for consistency. :/

Majkl578 · 2017-08-25T18:22:18Z

ping for 7.2

Ocramius · 2017-08-25T18:25:27Z

@Majkl578 the window for 7.2 is already closed, so this can sadly only land in 7.3

Majkl578 · 2017-08-25T18:30:27Z

7.2 isn't in RC phase afaik, is it? Should be fine then.
Anyway, 4 months for such simple addition... :(

nikic · 2017-08-25T20:24:58Z

Merged as d9d13ab into master.

Feature freeze is at beta, not RC. However, this kind of addition can still go in under the "small self-contained feature addition" clause, if the RMs approve. @remicollet @sgolemon What do you think about landing this in 7.2?

Majkl578 · 2017-09-08T01:21:12Z

@nikic: Thanks! 🎉

Last try before giving up - @remicollet @sgolemon would this be eligible for freeze exception as a small self-contained addition? Thanks.

Rican7 · 2018-01-25T02:05:40Z

Oh wow. I'm just now seeing this.

I want to thank @Majkl578 for resurrecting this, @derickr and @Ocramius for reviewing this, and to @nikic for merging this. I also really appreciate the contribution credit received and noted in the NEWS file.

Honestly, I was quite disheartened with the PHP contribution process nearly 3 years ago when I had originally created the #1145 PR. It was my first attempt at a code contribution to the PHP source, and I made quite an effort to make the contribution as clean and clear as possible, which was certainly a challenge with my incredibly limited C experience. Ultimately, the hasty actions taken by @smalyshev (2d9399a) with incredibly little communication based on a single message on the internals mailing list, was a really frustrating and unnecessary experience, as is now apparent by the merging of this recreated work.

Ultimately, I'm happy to see the code being merged in, and I appreciate the contribution credit, however I wish to make a plea with the internals team to reflect on why this user-friendly and simple change was now considered and merged in nearly 3 years and 4 minor releases later. I only make this plea to hope that we can further encourage contribution in the future, rather than seemingly discouraging it.

Thank you! 🙂

Additional context:

derickr · 2018-05-13T14:50:09Z

This addition has not been documented yet: http://php.net/manual/en/class.datetime.php — please add documentation.

ryanotella · 2018-05-22T03:34:39Z

I agree that the absence of this method blocks consistent transition of legacy projects to using immutable dates. TBH, it's been blocking us for several years now - every attempt has been reverted.

However, the method really needed to be in PHP 5.5 and not 7.3+.

Anyone supporting a legacy project is unlikely to upgrade to 7.3+ in order to benefit from the method, so it's hard to care.
Anyone running something edgish, probably isn't interested in legacy transitions, so it's hard to care.

And most people are one or the other most of the time, so nobody cares. I'd suggest that virtually nobody will benefit from this method for a couple of years. That doesn't mean it shouldn't happen.

It's available in PHP 7.3, see [FR 74668](https://bugs.php.net/bug.php?id=74668) and php/php-src#2484

ondrejmirtes · 2019-04-16T12:15:27Z

Hi, it seems like this method is missing from the documentation: https://www.php.net/manual/en/class.datetime.php

cmb69 · 2021-03-04T13:06:30Z

Hi, it seems like this method is missing from the documentation:

FTR, it has been documented in the meantime.

Majkl578 mentioned this pull request Apr 17, 2017

Feature - DateTime::createFromImmutable() method #1145

Merged

derickr requested changes Apr 17, 2017

View reviewed changes

Majkl578 force-pushed the datetime-createfromimmutable branch from 02970e8 to 810d40a Compare April 17, 2017 17:50

Majkl578 force-pushed the datetime-createfromimmutable branch from 810d40a to 96476ce Compare April 17, 2017 23:48

derickr requested changes Apr 18, 2017

View reviewed changes

krakjoe added the Feature label Apr 20, 2017

nikic added the Waiting on Author label May 1, 2017

Added method DateTime::createFromImmutable()

495b260

Majkl578 force-pushed the datetime-createfromimmutable branch from 96476ce to 495b260 Compare May 28, 2017 21:02

Ocramius approved these changes Jul 3, 2017

View reviewed changes

nikic closed this Aug 25, 2017

Majkl578 deleted the datetime-createfromimmutable branch September 8, 2017 01:15

Majkl578 mentioned this pull request Jul 17, 2018

Add methods DateTime::createFrom and DateTimeImmutable::createFrom #3394

Closed

weirdan added a commit to weirdan/psalm that referenced this pull request Jan 16, 2019

Added DateTime::createFromImmutable()

c6d5451

It's available in PHP 7.3, see [FR 74668](https://bugs.php.net/bug.php?id=74668) and php/php-src#2484

weirdan mentioned this pull request Jan 16, 2019

Added DateTime::createFromImmutable() vimeo/psalm#1206

Merged

muglug pushed a commit to vimeo/psalm that referenced this pull request Jan 17, 2019

Added DateTime::createFromImmutable()

0358719

It's available in PHP 7.3, see [FR 74668](https://bugs.php.net/bug.php?id=74668) and php/php-src#2484

Added method DateTime::createFromImmutable() [follow-up for #1145] #2484

Added method DateTime::createFromImmutable() [follow-up for #1145] #2484

Uh oh!

Conversation

Majkl578 commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 commented Apr 17, 2017

Uh oh!

Majkl578 commented Apr 17, 2017

Uh oh!

hikari-no-yume commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enumag commented Apr 17, 2017

Uh oh!

Majkl578 commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enumag commented Apr 18, 2017

Uh oh!

drAlberT commented May 19, 2017

Uh oh!

DennisBirkholz commented May 28, 2017

Uh oh!

Majkl578 commented May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jun 23, 2017

Uh oh!

derickr commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 commented Aug 25, 2017

Uh oh!

Ocramius commented Aug 25, 2017

Uh oh!

Majkl578 commented Aug 25, 2017

Uh oh!

nikic commented Aug 25, 2017

Uh oh!

Majkl578 commented Sep 8, 2017

Uh oh!

Rican7 commented Jan 25, 2018

Uh oh!

derickr commented May 13, 2018

Uh oh!

hikari-no-yume commented Apr 17, 2017 •

edited

Loading

Majkl578 commented Apr 17, 2017 •

edited

Loading

Majkl578 commented May 28, 2017 •

edited

Loading